Shorten path names when --debug is not passed #20

KillTheMule · 2017-12-06T10:53:35Z

Closes #19.

I did a bit more, now the output looks like

struct.InlineString.html: Linked file primitive.char.html does not exist!
struct.InlineString.html: Linked file iter/trait.DoubleEndedIterator.html does not exist!
struct.InlineString.html: Linked file primitive.char.html does not exist!
struct.InlineString.html: Linked file iter/trait.DoubleEndedIterator.html does not exist!
struct.InlineString.html: Linked file primitive.char.html does not exist!
struct.InlineString.html: Linked file primitive.char.html does not exist!
struct.InlineString.html: Linked file primitive.char.html does not exist!
struct.InlineString.html: Linked file primitive.char.html does not exist!
struct.InlineString.html: Linked file primitive.char.html does not exist!
struct.InlineString.html: Linked file iter/trait.DoubleEndedIterator.html does not exist!
struct.InlineString.html: Linked file primitive.char.html does not exist!
struct.InlineString.html: Linked file primitive.char.html does not exist!

As you requested, I left the verbose output with the long file names. It's even longer now that I've added the origin of the checked link, but imho that's useful. You don't implement checking http urls, so I wasn't sure how to handle those things for http checking, so I stayed with simply passing along pathes.

I refrained from running rustfmt on it or doing any formatting on my own, so this can be reviewed more easily. Let me know if that floats your boat :)

KillTheMule · 2017-12-06T11:28:36Z

Ah, one more comment. I wasn't sure about this, you're moving away the config value for the canonicalized dir path, and pass that on. The Arg structure still contains the "unresolved" path, so I needed to pass on both. I don't know where you want to be going with this and what your design ideas are, but my suggestion would be to replace the arg_directory entry with the canonicalized path and only pass on the args structure, so that every function "down the chain" works with exactly the same path.

Let me know if you want me to change that.

hobofan · 2018-02-20T10:48:02Z

@KillTheMule So sorry that I haven't taken a look at it yet! It's totally okay to @mention me if it looks like I've forgotten about something.

Sadly merging #22 introduced some merge conflicts for this PR. I'll try to resolve them and the the PR merged ASAP!

KillTheMule · 2018-02-20T18:22:36Z

No worries :) If you have trouble, feel free to close this, I might be able to reproduce it on current master some time.

jyn514

Hello and sorry for the long delay in reviewing. This looks on the right track, I just had a few nitpicks. Do you mind rebasing this and addressing the comments?

If you're up for a challenge, it would also be great to have tests for the new feature: I think tests/simple_project would be a good place. You'll need to add a new broken link, then capture the output from cargo-deadlinks and make sure it has the filename you expect.

jyn514 · 2020-10-16T17:52:06Z

src/check.rs

+    if  args.flag_debug {
+      if path.exists() {
+          debug!("{}: Linked file {} exists.", file.display(), path.display());
+          true
+      } else {
+          error!("{}: Linked file {} does not exist!", file.display(), path.display());
+          false
+      }
    } else {
-        error!("Linked file at path {} does not exist!", path.display());
-        false
+      let shortpath = path.strip_prefix(base_dir).unwrap_or_else(|_| &path);
+      let shortfile = file.strip_prefix(base_dir).unwrap_or_else(|_| &file);
+      if path.exists() {
+          debug!("{}: Linked file {} exists.", shortfile.display(), shortpath.display());
+          true
+      } else {
+          error!("{}: Linked file {} does not exist!", shortfile.display(), shortpath.display());
+          false
+      }


Rather than duplicating this, can you lift the short* variables out of the if? Something like

let shortpath = if !args.flag_debug { file = file.strip_prefix(base_dir).unwrap_or(&file); path.strip_prefix(base_dir).unwrap_or(&path) } else { path };

Then you wouldn't need to write the path.exists() if block twice.

jyn514 · 2020-10-16T17:54:27Z

src/main.rs

@@ -42,7 +42,7 @@ Options:
 ";

 #[derive(Debug, RustcDecodable)]
-struct MainArgs {
+pub struct MainArgs {


You don't need this to be pub I think, check is a submodule.

Suggested change

pub struct MainArgs {

struct MainArgs {

jyn514 · 2020-10-16T17:55:01Z

src/main.rs

@@ -56,9 +56,9 @@ fn main() {

    init_logger(&args);

-    let dir = args.arg_directory.map_or_else(determine_dir, |dir| PathBuf::from(dir));
+    let dir = args.arg_directory.clone().map_or_else(determine_dir, |dir| PathBuf::from(dir));


You can get rid of the clone() if you change walk_dir to take a boolean instead of all of MainArgs. Alternatively you could change this to a &Path instead of a PathBuf.

KillTheMule · 2020-10-16T19:48:43Z

Ha very cool! I‘ll Look into it, but It‘ll probably be next week or so. Thanks for your comments!

KillTheMule · 2020-10-20T13:23:14Z

Ok, I rewrote this, based on #80. It doesn't really need #80, so let me know if you want that, otherwise I'll just remove the change to the internal arg handling.

I could use a hand on the test though, it passes locally, but seems like target/doc is not generated for simple_project on CI. Shouldn't it be by virtue of calling assert_doc?

jyn514 · 2020-10-20T17:32:58Z

(I'm going to address #80 here because I don't think it makes sense to merge independently of this.)

When working on rebasing #20 I found this. I don't really see why one would want CheckContext to exist. It's not a performance optimization (we're passing along a reference to a struct no matter what), we need an additional step for conversion, and we pass along less information after all. I'll need more detailed command line args information for #20, so I figured I could just enlargen CheckContext... but what's the point really? Lets stick to one argument struct which we'll pass read-only along the chain, I'd say.

I would prefer not to make the arguments struct part of the library; most of them don't seem relevant to calling deadlinks programatically. I think it also makes sense to strip paths in the binary part of the project, not the library. So my suggested approach is to make this change in main.rs, not in check_file_url:

cargo-deadlinks/src/main.rs

Line 174 in 117c396

error!("{}", err);

Instead of printing the error directly, inspect the file path and call strip_path there, not in the library itself. That way, other people using deadlinks as a crate still have the absolute path if they need it.

KillTheMule · 2020-10-21T09:55:32Z

Ok, I changed the code so basically everything is handled in main, except we need to put the path into the error struct, and the debug flag in the context struct.

The whole thing feels a tad clunky, because we're somewhat duplicating the printout code. I could make a method FileError.shorten() that returns a new File error with the pathes shortened, which can then use the regular Display implementation, but that will result in copying stuff around. We could make an intermediate struct that borrows from FileError, but that again will complicate the code quite a bit... let me know what you think!

Tests pass locally, as I said, but not on CI, will still need help with that.

jyn514 · 2020-10-22T01:09:15Z

The whole thing feels a tad clunky, because we're somewhat duplicating the printout code.
I could make a method FileError.shorten() that returns a new File error with the pathes shortened

Could you instead have a print_shortened that takes prefix: Option<PathBuf>? Then Display would delegate to print_shortened(None) and the main binary would use print_shortened(Some(basedir)). As a bonus, you no longer need basedir in FileError - it seems silly to store it in each and every error.

we need to put the path into the error struct, and the debug flag in the context struct

Can you instead use the MainArgs struct in walk_dir? Then we keep the clean separation where the library doesn't need to know anything about shortening paths.

To avoid borrow-check errors, you may need to change into() to as_ctx(), which I'm imagining as impl MainCtxt { fn as_ctx(&self) -> CheckContext { ... }}. Alternatively you could just directly construct the context without a helper method.

jyn514 · 2020-10-22T01:20:44Z

Hmm, I can reproduce the failures both locally and on master ... let me look into that, not sure how it slipped past CI.

jyn514 · 2020-10-22T01:28:06Z

The issue you're running into is that the tests can't run in parallel; remove_all removes the same directory for both tests, so files are being removed as they're being checked. The fix is to add to the existing #[test] instead of adding a new one; I'll try to think of a way to allow running in parallel without having to make a new cargo directory for each test.

The issue I hit was specific to CARGO_TARGET_DIR and I fixed it in b4f091b (it was removing simple_project/target, but the target directory was actually $CARGO_TARGET_DIR/simple_project).

KillTheMule · 2020-10-22T10:09:20Z

Next round :) Inlined the test, and constructed the CheckContext right before passing it into the lib.

I didn't see how I could pass a formatter to print_shortened outside of the Display impl, so I had it return a String instead. I'll look around to see if there's a better solution.

KillTheMule · 2020-10-22T10:21:48Z

Hmm, I don't understand that test failure (passes locally, of course...). The output correctly points out the missing fn.bar.html in several files, and it also seems to be looking at struct.Tmp.html, but the broken link in the latter to the former is not reported.

I've been able to make this pass by expecting another file where the link is broken. Still feels brittle.

jyn514

I didn't see how I could pass a formatter to print_shortened outside of the Display impl, so I had it return a String instead.

Sure, that seems fine.

Hmm, I don't understand that test failure (passes locally, of course...)

Are you using nightly locally? Intra-doc links won't be stable for another 4 weeks. Your current test looks fine though.

Overall this looks great :) Just a small nit then I think this is ready to go.

src/lib.rs

When --debug is given, still show the full path

KillTheMule · 2020-10-22T13:28:49Z

Are you using nightly locally?

Riiight, man, the things one forgets :D Works out then!

jyn514 · 2020-10-22T13:31:03Z

Thanks so much for working on this! I'll try to get out a 0.4.2 release today so you can start using it right away :)

KillTheMule · 2020-10-22T13:32:49Z

Pleasure to do so, and thanks for your patient comments!

hobofan self-requested a review December 6, 2017 20:49

hobofan self-assigned this Jan 9, 2019

hobofan removed their assignment Oct 14, 2020

hobofan removed their request for review October 14, 2020 19:19

jyn514 reviewed Oct 16, 2020

View reviewed changes

jyn514 self-assigned this Oct 16, 2020

jyn514 added the C-enhancement Category: This is a new feature label Oct 16, 2020

jyn514 added this to the 0.4.2 milestone Oct 18, 2020

KillTheMule mentioned this pull request Oct 19, 2020

Remove CheckContext struct, put args into the lib #80

Closed

KillTheMule force-pushed the shorten branch from f0057be to 971aa09 Compare October 20, 2020 13:22

KillTheMule force-pushed the shorten branch 4 times, most recently from b5abf9a to 117c396 Compare October 20, 2020 13:34

KillTheMule force-pushed the shorten branch from 117c396 to be0e5b0 Compare October 21, 2020 09:54

KillTheMule force-pushed the shorten branch from be0e5b0 to f6bc52f Compare October 22, 2020 10:04

jyn514 approved these changes Oct 22, 2020

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

Print out errors without the full path

4d04f07

When --debug is given, still show the full path

KillTheMule force-pushed the shorten branch from b46786c to 4d04f07 Compare October 22, 2020 13:27

jyn514 merged commit 5d441cc into deadlinks:master Oct 22, 2020

KillTheMule deleted the shorten branch October 22, 2020 13:32

jyn514 changed the title ~~Improve display of findings~~ Shorten path names when --debug is not passed Oct 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shorten path names when --debug is not passed #20

Shorten path names when --debug is not passed #20

KillTheMule commented Dec 6, 2017

KillTheMule commented Dec 6, 2017

hobofan commented Feb 20, 2018

KillTheMule commented Feb 20, 2018

jyn514 left a comment

jyn514 Oct 16, 2020

jyn514 Oct 16, 2020

jyn514 Oct 16, 2020

KillTheMule commented Oct 16, 2020

KillTheMule commented Oct 20, 2020 •

edited

Loading

jyn514 commented Oct 20, 2020 •

edited

Loading

KillTheMule commented Oct 21, 2020 •

edited

Loading

jyn514 commented Oct 22, 2020

jyn514 commented Oct 22, 2020

jyn514 commented Oct 22, 2020 •

edited

Loading

KillTheMule commented Oct 22, 2020

KillTheMule commented Oct 22, 2020 •

edited

Loading

jyn514 left a comment

KillTheMule commented Oct 22, 2020

jyn514 commented Oct 22, 2020

KillTheMule commented Oct 22, 2020

Shorten path names when --debug is not passed #20

Shorten path names when --debug is not passed #20

Conversation

KillTheMule commented Dec 6, 2017

KillTheMule commented Dec 6, 2017

hobofan commented Feb 20, 2018

KillTheMule commented Feb 20, 2018

jyn514 left a comment

Choose a reason for hiding this comment

jyn514 Oct 16, 2020

Choose a reason for hiding this comment

jyn514 Oct 16, 2020

Choose a reason for hiding this comment

jyn514 Oct 16, 2020

Choose a reason for hiding this comment

KillTheMule commented Oct 16, 2020

KillTheMule commented Oct 20, 2020 • edited Loading

jyn514 commented Oct 20, 2020 • edited Loading

KillTheMule commented Oct 21, 2020 • edited Loading

jyn514 commented Oct 22, 2020

jyn514 commented Oct 22, 2020

jyn514 commented Oct 22, 2020 • edited Loading

KillTheMule commented Oct 22, 2020

KillTheMule commented Oct 22, 2020 • edited Loading

jyn514 left a comment

Choose a reason for hiding this comment

KillTheMule commented Oct 22, 2020

jyn514 commented Oct 22, 2020

KillTheMule commented Oct 22, 2020

KillTheMule commented Oct 20, 2020 •

edited

Loading

jyn514 commented Oct 20, 2020 •

edited

Loading

KillTheMule commented Oct 21, 2020 •

edited

Loading

jyn514 commented Oct 22, 2020 •

edited

Loading

KillTheMule commented Oct 22, 2020 •

edited

Loading